Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

implement url translation for legacy urls #1989

Merged
merged 1 commit into from
Sep 28, 2021
Merged

implement url translation for legacy urls #1989

merged 1 commit into from
Sep 28, 2021

Conversation

C0rby
Copy link
Contributor

@C0rby C0rby commented Aug 12, 2021 •
edited
Loading

First draft of the URL translation for OC10 URLS.
This implements a redirect from the old OC10 URL format to the current format we use in oCIS web. Currently only the public link and the old files app URL work because we don't do any file lookups by id yet. If this needs to be supported we would need to figure out how to lookup file based on their OC10 id.

To test this you need to apply this diff to oCIS and add a replace for reva to the go.mod file:

diff --git a/proxy/pkg/proxy/proxy.go b/proxy/pkg/proxy/proxy.go
index b066e9631..1d73b08b5 100644
--- a/proxy/pkg/proxy/proxy.go
+++ b/proxy/pkg/proxy/proxy.go
@@ -328,6 +328,10 @@ func defaultPolicies() []config.Policy {
                                        Endpoint: "/index.php/",
                                        Backend:  "http://localhost:9140",
                                },
+                               {
+                                       Endpoint: "/apps/files/",
+                                       Backend:  "http://localhost:9140",
+                               },
                                {
                                        Endpoint: "/data",
                                        Backend:  "http://localhost:9140",

@update-docs
Copy link

update-docs bot commented Aug 12, 2021

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@exalate-issue-sync exalate-issue-sync bot mentioned this pull request Aug 12, 2021
Closed
4 tasks
@lgtm-com
Copy link

lgtm-com bot commented Aug 12, 2021

This pull request introduces 1 alert when merging 3c1f98c into 756bdce - view on LGTM.com

new alerts:

  • 1 for Open URL redirect

@C0rby C0rby force-pushed the legacy-path-translations branch from 3c1f98c to c9bbb4d Compare August 25, 2021 14:40
@lgtm-com
Copy link

lgtm-com bot commented Aug 25, 2021

This pull request introduces 1 alert when merging c9bbb4d into 011bc08 - view on LGTM.com

new alerts:

  • 1 for Open URL redirect

@C0rby
Copy link
Contributor Author

C0rby commented Sep 8, 2021

@labkode @butonic, this PR implements the OC10 URL redirect to oCIS.
Currently only index.php/s/<token> (public links) and apps/files/?dir=/Documents&fileid=32 are supported. But for the second URL only the dir parameter is used.

Is the lookup by fileid required? If so, how should we implement this? Do we need to access the OC10 database or is that information accessible somewhere in reva?

@C0rby C0rby force-pushed the legacy-path-translations branch 2 times, most recently from c8f87a2 to 8ac6d09 Compare September 27, 2021 11:35
@lgtm-com
Copy link

lgtm-com bot commented Sep 27, 2021

This pull request introduces 1 alert when merging 8ac6d09 into 969ac3c - view on LGTM.com

new alerts:

  • 1 for Open URL redirect

@C0rby C0rby force-pushed the legacy-path-translations branch from 8ac6d09 to 73c8aee Compare September 27, 2021 11:53
@lgtm-com
Copy link

lgtm-com bot commented Sep 27, 2021

This pull request introduces 1 alert when merging 73c8aee into 969ac3c - view on LGTM.com

new alerts:

  • 1 for Open URL redirect

@C0rby
Copy link
Contributor Author

C0rby commented Sep 27, 2021

To move forward with this I would propose to merge this PR in this state and if necessary implement the fileid based lookup later on.

This PR will allow the following:

  • oc10.example.com/apps/files/?dir=/Photos&fileid=10 -> ocis.example.com/#/files/list/all/Photos
  • oc10.example.com/s/TttylAos50jEApg -> ocis.example.com/#/files/public/list/TttylAos50jEApg

Also I think the lgtm bot alert can be ignored since this is not an open redirect.
The target URL is always fixed to the configured public URL.
So the user will either get redirected to:

  • a login
  • the requested path
  • a not found page

@C0rby C0rby force-pushed the legacy-path-translations branch from 73c8aee to a8f6ef8 Compare September 27, 2021 12:17
@C0rby C0rby marked this pull request as ready for review September 27, 2021 12:18
@C0rby C0rby requested a review from labkode as a code owner September 27, 2021 12:18
@C0rby C0rby requested review from butonic, refs and ishank011 September 27, 2021 12:19
@C0rby C0rby self-assigned this Sep 27, 2021
@lgtm-com
Copy link

lgtm-com bot commented Sep 27, 2021

This pull request introduces 1 alert when merging a8f6ef8 into 969ac3c - view on LGTM.com

new alerts:

  • 1 for Open URL redirect

@labkode labkode merged commit 5fbab24 into cs3org:master Sep 28, 2021
@C0rby C0rby deleted the legacy-path-translations branch September 28, 2021 07:36
glpatcern pushed a commit to glpatcern/reva that referenced this pull request Nov 12, 2021
@glpatcern
implement url translation for legacy urls (cs3org#1989)
93144d9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refs refs refs approved these changes

@labkode labkode labkode approved these changes

@butonic butonic Awaiting requested review from butonic

@ishank011 ishank011 Awaiting requested review from ishank011

Assignees

@C0rby C0rby

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@C0rby @labkode @refs